fix: data races in tests#440
Merged
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 39 minutes and 3 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR refactors all Cobra command definitions from package-level variables with init()-based registration to factory functions, eliminating global state races during concurrent command instantiation. Monorepo subcommands use scratch variables populated per-invocation to safely bind flags. Testing and CI now include race-condition detection. ChangesCobra command factory refactoring for concurrent safety
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/dev/ci/monorepo/run.go (1)
94-96:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
modeparameter instead of package-levelrunModein error message.The error message references the package-level scratch variable
runModeinstead of the function parametermode. This is inconsistent with the rest of the function and could produce misleading error messages in tests that callrunWrapperdirectly.Proposed fix
default: - return fmt.Errorf("unknown runMode: %s", runMode) + return fmt.Errorf("unknown runMode: %s", mode) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/dev/ci/monorepo/run.go` around lines 94 - 96, The default case in runWrapper currently uses the package-level variable runMode in the error string; update the error to reference the function parameter mode instead. Locate the switch/default inside runWrapper and change the fmt.Errorf("unknown runMode: %s", runMode) call to fmt.Errorf("unknown runMode: %s", mode) so the error message reflects the actual argument passed to runWrapper rather than the package-level scratch variable.
🧹 Nitpick comments (1)
cmd/dev/ci/monorepo/components.go (1)
17-19: ⚡ Quick winRemove the
componentModepackage-level scratch variable and switch onlocalComponentModedirectly.
componentModeis only used insidenewComponentsCmd()’sRunclosure incmd/dev/ci/monorepo/components.go, so the package-level mutable state is unnecessary. Eliminating it avoids any residual shared-state risk while keeping the behavior the same.♻️ Proposed change
-// Package-level scratch variable for the `components` subcommand, populated -// from a per-command local in Run so concurrent construction does not race. -var componentMode string - func newComponentsCmd() *cobra.Command { var localComponentMode string c := &cobra.Command{ Use: "components", Short: "List components based on mode.", Long: `List components based on mode by reading dependency configs and displaying the dependency graph.`, Run: func(cmd *cobra.Command, args []string) { - componentMode = localComponentMode - var graph ComponentGraph _, _ = graph.getComponentGraph(rootDirectory) - switch componentMode { + switch localComponentMode { case "affected": affectedComponents := getAffectedComponents(&graph) displayComponents(affectedComponents) case "all": allComponents := graph.components displayComponents(allComponents) case "changed": changedComponents := getChangedComponents(&graph) displayComponents(changedComponents) case "involved": involvedComponents := getInvolvedComponents(&graph) displayComponents(involvedComponents) default: - log.Fatalf("Unknown ListMode '%s'", componentMode) + log.Fatalf("Unknown ListMode '%s'", localComponentMode) } }, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/dev/ci/monorepo/components.go` around lines 17 - 19, Remove the package-level variable componentMode and use the local variable localComponentMode directly inside the Run closure of newComponentsCmd; update any switch/case or conditional that references componentMode to instead reference localComponentMode (inside newComponentsCmd and its Run), and delete the now-unused componentMode declaration to eliminate unnecessary package-level mutable state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/dev/ci/github/env.go`:
- Around line 54-59: The guard incorrectly treats an unset/empty
SWAGGER_SPEC_IGNORE_PKGS as non-empty because strings.Split("", ",") yields
[""], causing export of "-x " entries; update the logic around reading
SWAGGER_SPEC_IGNORE_PKGS (the os.Getenv call and ignorePkgs handling) to first
check if the env var is non-empty (or filter out empty strings after
strings.Split) before building the "-x " list and calling fmt.Printf, ensuring
ignorePkgs contains only non-empty package names before joining and printing.
In `@cmd/dev/ci/monorepo/main.go`:
- Line 18: Remove the unused scratch variable "branch" and its flag binding from
the monorepo command or wire it into downstream logic; specifically, delete the
"branch string" declaration and the corresponding "--branch" PersistentFlag
setup in main.go (the PersistentPreRun populates it) OR propagate the variable
into the existing flow (e.g., pass branch into the functions used by
run.go/changes.go or use it inside isPR()/change-detection logic) so the value
is consumed rather than unused.
---
Outside diff comments:
In `@cmd/dev/ci/monorepo/run.go`:
- Around line 94-96: The default case in runWrapper currently uses the
package-level variable runMode in the error string; update the error to
reference the function parameter mode instead. Locate the switch/default inside
runWrapper and change the fmt.Errorf("unknown runMode: %s", runMode) call to
fmt.Errorf("unknown runMode: %s", mode) so the error message reflects the actual
argument passed to runWrapper rather than the package-level scratch variable.
---
Nitpick comments:
In `@cmd/dev/ci/monorepo/components.go`:
- Around line 17-19: Remove the package-level variable componentMode and use the
local variable localComponentMode directly inside the Run closure of
newComponentsCmd; update any switch/case or conditional that references
componentMode to instead reference localComponentMode (inside newComponentsCmd
and its Run), and delete the now-unused componentMode declaration to eliminate
unnecessary package-level mutable state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a9f04a2a-9a62-468a-86d5-2ec0fa88be67
📒 Files selected for processing (39)
.github/workflows/ci.yamlMakefilecmd/dev.gocmd/dev/ci/deps/main.gocmd/dev/ci/deps/url.gocmd/dev/ci/github/env.gocmd/dev/ci/github/main.gocmd/dev/ci/main.gocmd/dev/ci/monorepo/changes.gocmd/dev/ci/monorepo/components.gocmd/dev/ci/monorepo/main.gocmd/dev/ci/monorepo/run.gocmd/dev/ci/orbs/bump.gocmd/dev/ci/orbs/main.gocmd/dev/headers/copyright.gocmd/dev/headers/cp.gocmd/dev/headers/main.gocmd/dev/main.gocmd/dev/markdown/main.gocmd/dev/markdown/render.gocmd/dev/newsletter/draft.gocmd/dev/newsletter/main.gocmd/dev/newsletter/send.gocmd/dev/openapi/main.gocmd/dev/openapi/migrate.gocmd/dev/release/compile.gocmd/dev/release/main.gocmd/dev/release/notify/draft.gocmd/dev/release/notify/main.gocmd/dev/release/notify/send.gocmd/dev/release/publish.gocmd/dev/schema/main.gocmd/dev/schema/render_version.gocmd/dev/swagger/main.gocmd/dev/swagger/sanitize.gocmd/nodev.gocmd/root.gocmd/root_race_test.gocmd/version.go
💤 Files with no reviewable changes (1)
- Makefile
- golang.org/x/crypto v0.51.0 -> v0.53.0 (fixes multiple critical CVEs incl. GO-2026-5006, 5017, 5019, 5020, 5021, 5023, 5005) - golang.org/x/net v0.54.0 -> v0.55.0 (fixes GO-2026-5026 critical) - github.com/gomarkdown/markdown -> 20260417 (fixes GHSA-77fj-vx54-gvh7 high) Regenerates the newsletter render snapshot to absorb a one-blank-line cosmetic change in gomarkdown's HTML output.
Newer master commits of ory/x (and the ory/kratos / ory/hydra / ory/keto that consume it) no longer pull github.com/docker/docker through the x/sqlcon/dockertest chain, removing GO-2026-4883 (which has no fix version available upstream).
Calling page.Close() while a BrowserContext.onRoute handler is still
in flight is a data race inside playwright-go. Call UnrouteAll with
UnrouteBehaviorWait first so registered handlers complete before the
page is closed.
Also fixes a pre-existing guard bug in `dev ci github env`: an empty
SWAGGER_SPEC_IGNORE_PKGS would print "-x " because strings.Split("", ",")
returns [""] rather than nil. Check the raw env var first.
zepatrik
approved these changes
Jun 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Calling
cmdA.AddCommand(cmdB)modifies bothcmdAandcmdBand hence is not goroutine safe.Since we extensively test most commands and subcommands, we cannot have global
var ... = &cobra.Command{...}declarations.This diff is a lot more palatable when ignoring whitespace.
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores